chore(auth): drop direct user-permission grants — roles only - #366
Merged
Conversation
Closes the role-based permission epic (#142, phase 7 / #149). A permission reached a UserAccount three ways: a direct CongregationUserPermission row, a role on the account, or a role on its linked Member. The first path is now gone, so the settings role screens are the single place access is granted. Migration 20260826000000 turns every direct grant into a role assignment, then drops the table. Each (congregation, permission) pair that had at least one grant gets one auto-role granting exactly that permission, keyed can-<verb>-<subject> per AUTO_ROLE_KEY_BY_PERMISSION. They are ordinary custom roles afterwards — renameable, deletable — with a null name so the label resolves from the message catalogue rather than pinning a language into the database. A congregation that already owned a role under one of those keys keeps it untouched; the auto-role lands on <key>-migrated instead. Adopting the existing role would have silently widened it for everyone assigned to it. Behaviour preservation is pinned by an integration test that captures each account's effective permission set before the backfill and asserts it is identical after, across an admin held only directly, a user holding the same permission both directly and via a role, a member-bound role that must not be touched, an account with nothing, and the collision case. Also: - resolveEffectivePermissions and accountsWithPermissionFilter walk roles only; the three-path comment is now two. - setupFirstAccount and registerCongregation hand the first account admin through ensureAdminRole, inside the scoped transaction — the role tables are RLS-scoped, unlike the direct grant they replace. - updateAccount edits identity only. Its last-admin pre-check goes with the permission arm; setUserCustomRoleAssignments already guards role removal. - The archive importer keeps reading congregation-user-permissions.ndjson (and the pre-#152 congregation-user-roles.ndjson) and lands each grant on the matching auto-role, so older backups still restore everyone's access. Exports no longer emit the file. - The GDPR account export reports role-mediated permissions; anonymize drops the role assignments, which is what now revokes access. - The "Autorisations supplémentaires" card and the direct-permission count column are removed from the user screens. The control plane was migrated off this table first (Unitae/unitae-platform), since its CD applies this migration before rolling its own image.
… re-runnable Review follow-ups on the direct-grant cutover. The CI failure was a fourth issue: the migration test built its fixture in the table the migration drops, which exists locally but not in CI, where `prisma migrate deploy` runs first. - The backfill joined permissions to its mapping table with an inner join, so a permission that shipped between this file being written and being run would have had its grants dropped along with the table — permanently. It now LEFT JOINs with a `can-<key>` fallback, and a test grants an unmapped permission and asserts it survives. That test failed before the fix with an empty permission set, which is exactly the silent revocation being guarded against. - The file claimed to be re-runnable but was not: a second run saw the roles the first had created, took them for collisions and made duplicates, and filed a second audit event per congregation. The collision rule now reuses a role that grants exactly the one permission — which also makes the SQL agree with `resolveAutoRoleId` on the archive-import path — and the audit insert has an explicit re-run guard. Pinned by a new idempotence assertion. - The importer logged nothing when it skipped a grant, so a restore that returned less access than the archive held looked like a success. Both skip paths now warn, and an unmapped key gets the same fallback as the migration. - ensureAdminRole returning null meant a congregation could be provisioned with nobody able to administer it, silently. It now logs an error; provisioning still continues, since a half-created congregation is worse. Given a co-located test, which it lacked (test:service-test-coverage only scans app/features). - AUTO_ROLE_NAMES is keyed by AutoRoleKey rather than string, so adding a permission without a display name fails to compile instead of rendering the raw slug to an admin.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #149. Final phase of the role-based permission epic (#142).
A permission reached a
UserAccountthree ways: a directCongregationUserPermissionrow, a role on the account, or a role on its linked Member. This removes the first path, so the settings role screens become the single place access is granted — the epic's stated success criterion that no code path looks up a user's permissions directly.Migration
20260826000000_drop_direct_user_permissionsturns every direct grant into a role assignment, then drops the table. For each(congregation, permission)pair with at least one grant it creates one auto-role granting exactly that permission, keyedcan-<verb>-<subject>.Auto-roles are ordinary custom roles afterwards —
isBuiltIn: false, renameable, deletable — withnameanddescriptionleftNULL. That is the convention built-in roles already use:getRoleDisplayNameresolves the label from the message catalogue for the reader's locale, so no French or English text is pinned into the database, and an admin who renames one stores anamethat then wins.Collision guard. A congregation may already own a custom role slugified to
can-do-anything— an admin is free to name a role "Peut tout faire". Adopting it would silently grantadminto everyone already assigned to it. A taken key falls back to<key>-migrated, then<key>-migrated-<permissionId>.The test that matters
drop-direct-user-permissions.integration.test.tscaptures every account's effective permission set before the backfill and asserts it is identical after — the "every user keeps the same effective permissions" criterion, and the only thing standing between this change and a silent authorization regression. The fixture covers:can-do-anythinggranting something elseIt reads the shipped SQL file rather than a paraphrase, so re-typing the migration cannot make it pass.
One deliberate compromise: the test executes the whole backfill but not the final
DROP TABLE. Dropping takesACCESS EXCLUSIVEnot only on the table but on every table its foreign keys reference —UserAccount,Permission,Congregation— and the fixture holds that until rollback. Integration files run in parallel against one database, so executing it deadlocked unrelated suites at random. The drop is instead asserted as the migration's last statement; it is one self-evident statement, while the backfill is the part that can be wrong. Verified stable across three consecutive full integration runs.Everything else
resolveEffectivePermissionsandaccountsWithPermissionFilterwalk roles only. The file's three-path header comment is now two.setupFirstAccount/registerCongregationhand the first account admin throughensureAdminRole, moved inside the scoped transaction —Role,RolePermissionandUserRoleAssignmentare RLS-scoped, unlike the unscoped direct grant they replace.updateAccountedits identity only. ItsrequireNotLastAdminpre-check goes with the permission arm — that guard (and its stale "false positive" comment) is now redundant, sincesetUserCustomRoleAssignmentsalready blocks removing an Admin-granting role from the last admin.importCongregationUserPermissionsstill readscongregation-user-permissions.ndjson(and the pre-chore(auth): rename UserRole to Permission #152congregation-user-roles.ndjson) and lands each grant on the matching auto-role, same collision rule as the migration — so restoring a pre-cutover backup doesn't silently drop everyone's access. Exports no longer emit the file; roles, role-permissions and role assignments travel as their own entities.anonymizeAccountdrops the role assignments, which is what now revokes access.edit-role.tsxprefills the name field fromgetRoleDisplayNameso renaming an auto-role starts from its displayed name rather than an empty required field.permissions-and-roles.md(new Auto-roles section),architecture.md,notifications.md,data-transfer.md,row-level-security.md,CLAUDE.md.Notes on the issue text
The issue predated some of the code and needed three corrections, all applied:
can-<verb>-<subject>pattern and agreed before the migration was written./settings/users/:accountId/edit, not the publisher edit page (the UI refresh moved it). The user list also carried a "Permissions personnalisées" count fed by the same table.Deployment
The control plane also reads this table, and was migrated off it first. Deployment order therefore matters; the coordination is tracked separately.
Testing
Written test-first throughout.
pnpm test:unit(3154),test:integration(341),test:typecheck,test:lint,test:boundaries,test:aggregate-boundaries,test:tenant-scoping,test:server-barrel-exports,test:service-test-coverage,test:file-sizesandpnpm buildall green.Not done: a manual UI pass against a migrated local database.